-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove indirection for builtin operator tests #2109
Conversation
The idea of these tests was to make sure these interfaces are properly implemented for builtin types. The revised tests would pass if the interfaces don't work for builtin types but the operators are instead special-cased for those types. |
Can you explain to me how that differs from the existing template code? How does the existing template code force the i32 builtin to be used? (that is, how does it prevent writing an interface for the type if the alternative doesn't?) |
I think I'm starting to understand what you mean, but this makes me wonder if this is behaving as intended... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks like a simpler and clearer way of testing the same thing.
I think the problem here was that you're asking for a |
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
@zygoloid Pinging here, did you have any remaining concerns? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional suggestions to make tests less likely to pass by accident, but LG either way.
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Not sure if there was something behind the indirection...?